assert: typed array deepequal performance fix #4330

claudiorodriguez · 2015-12-17T13:09:13Z

assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

Fixes: #4294

jasnell · 2015-12-17T16:26:11Z

LGTM

cjihrig · 2015-12-17T16:45:20Z

lib/assert.js

@@ -143,6 +143,12 @@ assert.deepStrictEqual = function deepStrictEqual(actual, expected, message) {
 };

 function _deepEqual(actual, expected, strict) {
+  // If both values are instances of typed arrays, wrap them in
+  // a Buffer each to increase performance
+  if (ArrayBuffer.isView(actual) && ArrayBuffer.isView(expected)) {


Can this logic be pushed down further? At least past the "7.1." check on line 152.

cjihrig · 2015-12-17T16:49:18Z

The test should probably have some cases where the two values are not equal.

cjihrig · 2015-12-17T16:50:43Z

This seems fine, but I'd like feedback from other collaborators. Are there any edge cases where this change could come back to bite us?

claudiorodriguez · 2015-12-17T17:42:07Z

@cjihrig Thanks, just pushed the changes, +1 on waiting for feedback

Trott · 2015-12-17T19:24:10Z

lib/assert.js

@@ -170,7 +170,12 @@ function _deepEqual(actual, expected, strict) {
             (expected === null || typeof expected !== 'object')) {
    return strict ? actual === expected : actual == expected;

-  // 7.5 For all other Object pairs, including Array objects, equivalence is
+  // 7.5. If both values are instances of typed arrays, wrap them in


These numbers (7.5) are not arbitrary! They come from the CommonJS standard. Looks like they may already be goobered up, but that's not a reason to make them further-removed from the standard they are intended to refer to.

claudiorodriguez · 2015-12-17T19:43:41Z

@Trott Oops, thanks for catching that, fixed jslint issues.

claudiorodriguez · 2015-12-17T19:48:58Z

@Trott You really learn something new every day, thanks again.

Trott · 2015-12-17T20:36:32Z

Is it worth taking a benchmark to confirm the performance improvement? A benchmark that we could run across different operating systems and hardware on the CI server might be useful.

claudiorodriguez · 2015-12-17T21:06:36Z

@Trott you mean something like what I just pushed? Kinda copied over another benchmark's code. Comparison between assert with the fix and without it shows a massive improvement. (20 vs 0.08)

PS: jslint wasn't happy about the benchmark, in fact it isn't happy about a lot of files in the benchmark folder... I think benchmarks are a tad obsolete in general

Trott · 2015-12-17T21:31:49Z

@fansworld-claudio On the benchmark file: Yep, exactly. Thanks! 👍

And yes, the benchmark directory is not included in make jslint so, yeah, I guess it's not surprising that there's lots of stuff in there that isn't compliant with other parts of the code base. (Currently, make jslint lints src, lib, test, and tools/eslint-rules only.)

silverwind · 2015-12-18T00:17:29Z

Can we see some benchmark numbers for other primitve type? There should be a regression, hopefully small.

claudiorodriguez · 2015-12-18T00:32:28Z

@silverwind Good idea, I'll have it tomorrow. There should only be a very slight impact when asserting inside huge loops.

assert.deepEqual: when actual and expected are typed arrays, wrap them in a new Buffer each to increase performance significantly. Fixes: nodejs#4294

claudiorodriguez · 2015-12-18T12:55:12Z

@silverwind added two benchmarks:

big array objects (falling into 7.5):

before PR:

assert\deepequal-prims-and-objs-big-array.js prim=null n=25: 1.14118
assert\deepequal-prims-and-objs-big-array.js prim=undefined n=25: 1.16831
assert\deepequal-prims-and-objs-big-array.js prim=a n=25: 1.21148
assert\deepequal-prims-and-objs-big-array.js prim=1 n=25: 1.22213
assert\deepequal-prims-and-objs-big-array.js prim=true n=25: 1.14505
assert\deepequal-prims-and-objs-big-array.js prim=[object Object] n=25: 1.22749
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.23757
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.20910

after PR:

assert\deepequal-prims-and-objs-big-array.js prim=null n=25: 1.22064
assert\deepequal-prims-and-objs-big-array.js prim=undefined n=25: 1.20545
assert\deepequal-prims-and-objs-big-array.js prim=a n=25: 1.19943
assert\deepequal-prims-and-objs-big-array.js prim=1 n=25: 1.21435
assert\deepequal-prims-and-objs-big-array.js prim=true n=25: 1.14204
assert\deepequal-prims-and-objs-big-array.js prim=[object Object] n=25: 1.18320
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.19306
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.21798

The fluctuations seem to be within random variance (in my computer's case), so there doesn't seem to be a real change here.

big loops (forcing into 7.5 with array object):

before PR:

assert\deepequal-prims-and-objs-big-loop.js prim=null n=100000: 31422.79244
assert\deepequal-prims-and-objs-big-loop.js prim=undefined n=100000: 28421.44023
assert\deepequal-prims-and-objs-big-loop.js prim=a n=100000: 30207.53243
assert\deepequal-prims-and-objs-big-loop.js prim=1 n=100000: 29245.98661
assert\deepequal-prims-and-objs-big-loop.js prim=true n=100000: 25392.93322
assert\deepequal-prims-and-objs-big-loop.js prim=[object Object] n=100000: 29751.57255
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 29663.82391
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 32222.55574

after PR:

assert\deepequal-prims-and-objs-big-loop.js prim=null n=100000: 28553.61637
assert\deepequal-prims-and-objs-big-loop.js prim=undefined n=100000: 30250.17099
assert\deepequal-prims-and-objs-big-loop.js prim=a n=100000: 31414.15124
assert\deepequal-prims-and-objs-big-loop.js prim=1 n=100000: 31509.91023
assert\deepequal-prims-and-objs-big-loop.js prim=true n=100000: 31548.13352
assert\deepequal-prims-and-objs-big-loop.js prim=[object Object] n=100000: 31572.05442
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 31233.08257
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 31618.14617

Also seems to be within random variance, after running it a few times... at least running it on my computer.

silverwind · 2015-12-18T20:22:23Z

Yeah, looks like v8 is optimizing the extra if branch pretty well and your results look to be just fluctuations. Thanks for adding these benchmarks 👍

mcollina · 2015-12-21T08:48:36Z

LGTM

silverwind · 2015-12-21T15:42:00Z

LGTM

cjihrig · 2015-12-21T15:50:03Z

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/1037/

assert.deepEqual: when actual and expected are typed arrays, wrap them in a new Buffer each to increase performance significantly. PR-URL: #4330 Fixes: #4294 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

silverwind · 2015-12-21T17:04:12Z

Thanks! Landed in 6378622.

assert.deepEqual: when actual and expected are typed arrays, wrap them in a new Buffer each to increase performance significantly. PR-URL: nodejs#4330 Fixes: nodejs#4294 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

* http: - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377 - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482 * This release also includes several minor performance improvements: - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330 - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780 - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484 - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780 - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964 PR-URL: nodejs#4547

* http: - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377 - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482 * This release also includes several minor performance improvements: - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330 - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780 - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484 - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780 - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964 Refs: nodejs#4547 PR-URL: nodejs#4623 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

assert.deepEqual: when actual and expected are typed arrays, wrap them in a new Buffer each to increase performance significantly. PR-URL: #4330 Fixes: #4294 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

assert.deepEqual: when actual and expected are typed arrays, wrap them in a new Buffer each to increase performance significantly. PR-URL: nodejs#4330 Fixes: nodejs#4294 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

* http: - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377 - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482 * This release also includes several minor performance improvements: - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330 - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780 - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484 - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780 - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964 Refs: nodejs#4547 PR-URL: nodejs#4623 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Fishrock123 added the assert Issues and PRs related to the assert subsystem. label Dec 17, 2015

cjihrig reviewed Dec 17, 2015
View reviewed changes

claudiorodriguez force-pushed the assert-performance-typed-arrays branch 2 times, most recently from 83468ba to 10d83e5 Compare December 17, 2015 17:40

Trott reviewed Dec 17, 2015
View reviewed changes

claudiorodriguez force-pushed the assert-performance-typed-arrays branch from 10d83e5 to 94bb526 Compare December 17, 2015 19:42

claudiorodriguez force-pushed the assert-performance-typed-arrays branch from 94bb526 to 5b14d55 Compare December 17, 2015 19:47

claudiorodriguez force-pushed the assert-performance-typed-arrays branch from 5b14d55 to 8f6a57c Compare December 17, 2015 21:04

claudiorodriguez force-pushed the assert-performance-typed-arrays branch from 8f6a57c to f79654a Compare December 17, 2015 21:14

assert: typed array deepequal performance fix

a6a874b

assert.deepEqual: when actual and expected are typed arrays, wrap them in a new Buffer each to increase performance significantly. Fixes: nodejs#4294

claudiorodriguez force-pushed the assert-performance-typed-arrays branch from f79654a to a6a874b Compare December 18, 2015 12:52

silverwind closed this Dec 21, 2015

jasnell added the lts-watch-v4.x label Dec 24, 2015

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 13, 2016

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

addaleax mentioned this pull request Mar 26, 2016

assert: Check typed array view type in deepEqual #5910

Closed

3 tasks

addaleax mentioned this pull request Apr 11, 2016

assert: Check typed array view type in deepEqual (backport to 4.x) #6147

Closed

3 tasks

addaleax mentioned this pull request Jan 21, 2017

assert: enforce type check in deepStrictEqual #10282

Closed

3 tasks

leonardoadame mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#377

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#1015

Open

leonardoadame mentioned this pull request Feb 11, 2024

[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 leonardoadame/node#646

Open

aliscco mentioned this pull request Feb 12, 2024

[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 aliscco/alisco-node#1250

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: typed array deepequal performance fix #4330

assert: typed array deepequal performance fix #4330

claudiorodriguez commented Dec 17, 2015

jasnell commented Dec 17, 2015

cjihrig Dec 17, 2015

cjihrig commented Dec 17, 2015

cjihrig commented Dec 17, 2015

claudiorodriguez commented Dec 17, 2015

Trott Dec 17, 2015

claudiorodriguez commented Dec 17, 2015

claudiorodriguez commented Dec 17, 2015

Trott commented Dec 17, 2015

claudiorodriguez commented Dec 17, 2015

Trott commented Dec 17, 2015

silverwind commented Dec 18, 2015

claudiorodriguez commented Dec 18, 2015

claudiorodriguez commented Dec 18, 2015

silverwind commented Dec 18, 2015

mcollina commented Dec 21, 2015

silverwind commented Dec 21, 2015

cjihrig commented Dec 21, 2015

silverwind commented Dec 21, 2015

assert: typed array deepequal performance fix #4330

assert: typed array deepequal performance fix #4330

Conversation

claudiorodriguez commented Dec 17, 2015

jasnell commented Dec 17, 2015

cjihrig Dec 17, 2015

Choose a reason for hiding this comment

cjihrig commented Dec 17, 2015

cjihrig commented Dec 17, 2015

claudiorodriguez commented Dec 17, 2015

Trott Dec 17, 2015

Choose a reason for hiding this comment

claudiorodriguez commented Dec 17, 2015

claudiorodriguez commented Dec 17, 2015

Trott commented Dec 17, 2015

claudiorodriguez commented Dec 17, 2015

Trott commented Dec 17, 2015

silverwind commented Dec 18, 2015

claudiorodriguez commented Dec 18, 2015

claudiorodriguez commented Dec 18, 2015

silverwind commented Dec 18, 2015

mcollina commented Dec 21, 2015

silverwind commented Dec 21, 2015

cjihrig commented Dec 21, 2015

silverwind commented Dec 21, 2015